Add `connect()`, `disconnect()`, deprecate `setAutoReconnect()`, `resume()`, and `connected` #9439

scottn12 · 2022-03-10T20:44:41Z

For context on the reasoning behind these changes see: #8745

This PR adds connect() and disconnect() to Container and IContainer. These functions will combine to replace the functionality of Container.setAutoReconnect() (which is deprecated here). Furthermore, connect() is essentially a rename of Container.resume(), hence resume() has been deprecated as well.

In addition, IContainer.connected has been deprecated in favor of IContainer.connectionState. To make IFluidContainer consistent with these changes, connectionState has been added to IFluidContainer, and IFluidContainer.connected has been deprecated as well.

Follow up items:

Add connect() and disconnect() to IFluidContainer and IContainer
Add tests for the new functionality
Update documentation referencing deprecated APIs
Explore behavior if disconnect() is called while the container is attempting to connect

Closes #9396, #9167, #9744

…esume

msfluid-bot · 2022-03-10T21:04:25Z

⯅ @fluid-example/bundle-size-tests: +667 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	386.54 KB	386.54 KB	■ No change
containerRuntime.js	193.14 KB	193.14 KB	■ No change
loader.js	159.99 KB	160.64 KB	⯅ +667 Bytes
map.js	208.23 KB	208.23 KB	■ No change
matrix.js	295.74 KB	295.74 KB	■ No change
odspDriver.js	182.94 KB	182.94 KB	■ No change
odspPrefetchSnapshot.js	76.72 KB	76.72 KB	■ No change
sharedString.js	315.73 KB	315.73 KB	■ No change
Total Size	1.81 MB	1.81 MB	⯅ +667 Bytes

Baseline commit: 27eafd9

Generated by 🚫 dangerJS against 05271fb

vladsud · 2022-03-10T22:25:32Z

api-report/fluid-static.api.md

@@ -18,6 +18,16 @@ import { IFluidDataStoreFactory } from '@fluidframework/runtime-definitions';
 import { IFluidLoadable } from '@fluidframework/core-interfaces';
 import { TypedEventEmitter } from '@fluidframework/common-utils';

+// @public
+export namespace ConnectionState {


I'd really love to see us change names here:

"Connecting" state should be actually call "Connected".

"Connected" should be renamed to something else that clearly articulates actual state - "up-to-date & syncing changes".

Also, enums are expensive in JS, and I've heard advice to use union types only (as on line 29).

I'd also suggest to re-export - it's cheaper that way.

I copied these values from container-definitions in order to make IContianer and IFluidContainer consistent, however we can actually just import ConnectionState from container-definitions directly. As per changing the names, I believe that would be a breaking change, so we should probably do that in a separate PR.

It would be great to have discussion on naming here. If we agree to change names, then I'd rather see you use new names here (doing remapping), not to create more back compat debt.

"Connected" should be renamed to something else that clearly articulates actual state - "up-to-date & syncing changes".

@vladsud, I think Synced could be an adequate name. My only concern is that it does not emphasize that the container is actively syncing changes, however, I think most developers should be able to infer this.

@vladsud, on a related note, do you know why we created ConnectionState in container.ts instead of just importing the one defined in container-definitions?

I do not know, I'd guess someone added it there, likely as result of adding method to IContainer. Likely before that work there was no need to have it in definitions package.

@markfields for naming discussion. I'm fine with "Synced", but let's run it by wider audience.

For more naming discussion: #9677

vladsud · 2022-03-10T22:28:18Z

common/lib/container-definitions/src/loader.ts

+    /**
+     * Try to connect the container to the delta stream
+     */
+    connect?(): void;


Why not add new APIs on class, propagate and wait some time, and then add them here, but not make them optional?
Alternatively, we can make them non-optional from start - I do not see big problem here. As long as we are adding implementation in parallel, package bump would be noop in the future.

Yeah, that makes sense. I need to create a follow up PR to add connect() and disconnect() to IFluidContainer anyway, so we could just add in the functions to both IContainer and IFluidContainer in one PR.

packages/loader/container-loader/src/container.ts

vladsud · 2022-03-10T22:31:48Z

packages/loader/container-loader/src/container.ts

+    }
+
+    private connectInternal(args: IConnectionArgs) {
+        assert(!this.closed, "Attempting to connect() a closed DeltaManager");


I'd like to see code reuse with existing resume/setAutoReconnect flow to easier see where this logic is different from existing logic. Ideally there should be no differences and one implementation should call into another, right?

So basically have connect() call resumeInteranl() and setAutoReconnect(true)? If so, we should probably make setAutoReconnect() a private function (since we would no longer be removing it).

It does not matter which way you go (connect calling setAutoReconnect(true) or other way around). We can clean that later when we remove deprecated APIs (and have just one function), but for now it would be really great to see if new functionality differs from the previous one through code reuse. If you can't reuse code, that means it's not 1-to-1 reman, and it should be clearly spelled out for consumers such that they are not caught by surprise.

The reason why I wrote a new connectInernal() function is because I felt that what we wanted is an overlap between the current resume() and setAutoReconnect(true) functions. However, I didn't want to call those functions straight up because there is a bit of overlap between the two. For example, they both call end up calling this.connectToDeltaStream(). Although I don't think this would cause issues it did seem a bit messy so I ended up essentially combining the functions while trying to eliminate the overlap (which resulted in connectInternal()). So I do believe we could reuse code, but it would be a bit inefficient. Do you think this is a bad approach and we should just reuse the existing logic?

So I assume the guidance is for people to move from setAutoReconnect(true) to connect() calls.
The two obvious things I see that are different in flows:

There is a check for _attachState === AttachState.Attached && this.resumedOpProcessingAfterLoad in setAutoReconnect() that is missing here

There is manualReconnectInProgress tracking.

Second one is likely not interesting and can probably be removed (or added).
First part - I do not know, it feels like it should apply here (I'd look at history on when/why it was added)

Looks reasonable after latest changes.

I think you could call resumeInternal from the top of connectInternal now, would be nice to reduce code duplication (and resumeInternal looks like it will stick around since it's called from attach)

And totally optional, but consider pulling the if (currentMode !== mode) { ... } block into a helper, since it's now here in triplicate. And it's pretty crucial the three places stay in sync since they're messing with the same state.

+1 to putting manualReconnectInProgress back the way it was... Or if not, then just delete that variable because this was the only place it was set to true.

Probably should take a pass over the values used for connectionInitiationReason in logConnectionStateChangeTelemetry and see if they still make sense given the merging of resume and setAutoReconnect(true) from an API standpoint. I think I'm confused why "AutoReconnect" and "ManualReconnect" were distinguished by this case anyway...

I agree with your first two comments and will update the PR shortly to make these changes. Re: your last comment, since we merged resume() and setAutoReconnect(), I think it does make sense to just delete manualReconnectInProgress.

… container-definitions, dont use this.connected

api-report/container-loader.api.md

Co-authored-by: Mark Fields <[email protected]>

packages/loader/container-loader/src/container.ts

common/lib/container-definitions/src/loader.ts

markfields · 2022-04-04T22:49:52Z

packages/loader/container-loader/src/container.ts

+            // Note: no need to fetch ops as we do it preemptively as part of DeltaManager.attachOpHandler().
+            // If there is gap, we will learn about it once connected, but the gap should be small (if any),
+            // assuming that connect() is called quickly after initial container boot.
+            this.connectInternal({ reason: "DocumentOpenResume", fetchOpsFromStorage: false });


Let's think about what the reason should be. It used to be different for resume v. setAutoReconnect, now it will be the same.

@markfields, thoughts on DocumentConnect? This would match the DocumentOpen reason used when the container is initially created and tries to connect.

packages/loader/container-loader/src/container.ts

markfields

Left some comments for you to look at, but looks good overall! And no pushback on connect() and disconnect() names in #9677, so I think we're good to go on the API.

common/lib/container-definitions/src/loader.ts

Co-authored-by: Mark Fields <[email protected]>

skylerjokiel · 2022-04-05T17:53:27Z

packages/loader/container-loader/src/container.ts

@@ -946,6 +954,85 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
        }
    }

+    public connect() {


Please add documentation

These should actually just inherit the docs from the interface.

skylerjokiel · 2022-04-05T17:53:47Z

packages/loader/container-loader/src/container.ts

+        }
+    }
+
+    public disconnect() {


Please add documentation.

skylerjokiel · 2022-04-05T17:58:38Z

common/lib/container-definitions/src/loader.ts

     */
    setAutoReconnect?(reconnect: boolean): void;

    /**
     * Have the container attempt to resume processing ops
-     * @alpha
+     * @deprecated - 0.58, This API will be removed in 0.60.0
+     * Use `connect()` instead


I agree with @heliocliu and I'm not sure why this was removed from this PR? The goal of this PR is to introduce connect and disconnect at the Container level. Since we are enforcing using IContainer we are telling devs to use APIs as part of depreciation that aren't actually exposed.

Seems like the right step is to add them as optional as part of this PR and include the version in the Breaking.md that we will be making the breaking change. Then make them mandatory in next (or whatever breaking change we decide)

…l helper, add usageErrors

markfields · 2022-04-06T18:33:05Z

packages/loader/container-loader/src/container.ts

@@ -1645,8 +1614,6 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
            }
            if (this.firstConnection) {
                connectionInitiationReason = "InitialConnect";
-            } else if (this.manualReconnectInProgress) {
-                connectionInitiationReason = "ManualReconnect";


@vladsud -- Do you know if this distinction of "ManualReconnect" was leveraged much in telemetry analysis? How big a loss is this? I don't quite get the semantics of it honestly, but wanted to double-check.

Probably? It's useful to learn how quickly document reflects latest changes after user comes back to a tab that was inactive for very long time. That was the reason for this property on connection events. I think it's interesting data to have, but with these changes I think responsibility is pushed to consumers, and I'm fine with it. But we should look closer if consumers have everything, they need to be successful here.

markfields

🎂 LGTM!

skylerjokiel

Looks good 👍

This PR adds tests for the newly added `connect()` and `disconnect()` functions (see #9439). Tests are added to `@fluidframework/test-end-to-end-tests`. The tests first check that the connection state is changed when using `connect()`/`disconnect()`. Next, we ensure that op processing is paused after `disconnect()`, and resumes after `connect()`. Closes #9746

This PR adds tests for the newly added `connect()` and `disconnect()` functions (see microsoft#9439). Tests are added to `@fluidframework/test-end-to-end-tests`. The tests first check that the connection state is changed when using `connect()`/`disconnect()`. Next, we ensure that op processing is paused after `disconnect()`, and resumes after `connect()`. Closes microsoft#9746

This PR removes the APIs deprecated in 0.58 via #9439, which include `Container.setAutoReconnect()`, `Container.resume()`, `IContainer.connected`, and `IFluidContainer.connected`. Additionally, this PR replaces any remaining usages/references of the above APIs with their respective replacements (see #9167). Closes #9397

scottn12 added 2 commits March 10, 2022 15:21

add connect(), disconnect(), deprecate setAutoReconnect, connected, r…

26fcaf4

…esume

revert this.connect() to resume()

1126595

scottn12 requested a review from heliocliu March 10, 2022 20:44

github-actions bot added area: definitions area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues public api change Changes to a public API labels Mar 10, 2022

scottn12 linked an issue Mar 10, 2022 that may be closed by this pull request

Deprecate Container.setAutoReconnect(), Container.resume(), IContainer.connected, and IFluidContainer.connected. #9167

Closed

scottn12 requested review from ChumpChief, anthony-murphy and vladsud March 10, 2022 22:04

scottn12 marked this pull request as ready for review March 10, 2022 22:04

scottn12 requested review from a team as code owners March 10, 2022 22:04

vladsud reviewed Mar 10, 2022

View reviewed changes

packages/loader/container-loader/src/container.ts Outdated Show resolved Hide resolved

vladsud reviewed Mar 10, 2022

View reviewed changes

scottn12 added 3 commits March 11, 2022 11:40

remove connect/disconncet from IContainer, reuse ConnectionState from…

8b2e788

… container-definitions, dont use this.connected

update api-report

df6192c

assert attatch state is connected for connectInternal()

879400f

markfields mentioned this pull request Mar 18, 2022

Connectivity eventing improvements #7884

Closed

Merge branch 'main' into containerConnectionFunctions

660a70f

github-actions bot added the base: main PRs targeted against main branch label Mar 28, 2022

scottn12 requested review from markfields and vladsud March 30, 2022 19:01

markfields reviewed Mar 30, 2022

View reviewed changes

api-report/container-loader.api.md Show resolved Hide resolved

Update packages/framework/fluid-static/src/fluidContainer.ts

1ad1809

Co-authored-by: Mark Fields <[email protected]>

markfields reviewed Apr 4, 2022

View reviewed changes

packages/loader/container-loader/src/container.ts Show resolved Hide resolved

markfields reviewed Apr 4, 2022

View reviewed changes

common/lib/container-definitions/src/loader.ts Outdated Show resolved Hide resolved

markfields reviewed Apr 4, 2022

View reviewed changes

packages/loader/container-loader/src/container.ts Outdated Show resolved Hide resolved

markfields approved these changes Apr 4, 2022

View reviewed changes

markfields reviewed Apr 4, 2022

View reviewed changes

common/lib/container-definitions/src/loader.ts Outdated Show resolved Hide resolved

markfields mentioned this pull request Apr 4, 2022

Naming consistency for API/event surface related to Container's connection to delta stream #9677

Closed

scottn12 and others added 3 commits April 5, 2022 10:08

Update packages/loader/container-loader/src/container.ts

a8c083b

Co-authored-by: Mark Fields <[email protected]>

Update common/lib/container-definitions/src/loader.ts

c6bbc93

Co-authored-by: Mark Fields <[email protected]>

add @deprecated to Container class resume/setAutoReconnect

5a58350

vladsud approved these changes Apr 5, 2022

View reviewed changes

skylerjokiel suggested changes Apr 5, 2022

View reviewed changes

scottn12 linked an issue Apr 5, 2022 that may be closed by this pull request

Add connect() and disconnect() to Container and IContainer #9744

Closed

add to IContainer, update BREAKING.md, create setAutoReconnectInterna…

b7a4697

…l helper, add usageErrors

scottn12 requested a review from skylerjokiel April 5, 2022 20:09

markfields reviewed Apr 6, 2022

View reviewed changes

markfields approved these changes Apr 6, 2022

View reviewed changes

change connect reason to DocumentConnect

05271fb

skylerjokiel approved these changes Apr 7, 2022

View reviewed changes

scottn12 merged commit 9031425 into microsoft:main Apr 7, 2022

scottn12 deleted the containerConnectionFunctions branch April 7, 2022 22:41

vladsud mentioned this pull request Apr 8, 2022

Pending (in-flight) connection request may result in breaking invariants #9787

Closed

This was referenced Apr 22, 2022

Add tests for connect() and disconnect() #10009

Merged

Update documentation for connection APIs #9170

Closed

scottn12 mentioned this pull request May 6, 2022

Remove deprecated container connection APIs #10192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `connect()`, `disconnect()`, deprecate `setAutoReconnect()`, `resume()`, and `connected` #9439

Add `connect()`, `disconnect()`, deprecate `setAutoReconnect()`, `resume()`, and `connected` #9439

scottn12 commented Mar 10, 2022 •

edited

Loading

msfluid-bot commented Mar 10, 2022 •

edited

Loading

vladsud Mar 10, 2022

vladsud Mar 10, 2022

scottn12 Mar 11, 2022

vladsud Mar 11, 2022

scottn12 Mar 11, 2022

scottn12 Mar 14, 2022 •

edited

Loading

vladsud Mar 15, 2022

vladsud Mar 15, 2022

scottn12 Mar 30, 2022

vladsud Mar 10, 2022

scottn12 Mar 11, 2022

vladsud Mar 10, 2022

scottn12 Mar 11, 2022

vladsud Mar 11, 2022 •

edited

Loading

scottn12 Mar 11, 2022 •

edited

Loading

vladsud Mar 15, 2022

vladsud Apr 1, 2022

markfields Apr 4, 2022

markfields Apr 4, 2022

markfields Apr 4, 2022

scottn12 Apr 5, 2022

markfields Apr 4, 2022

scottn12 Apr 5, 2022 •

edited

Loading

markfields left a comment

skylerjokiel Apr 5, 2022

skylerjokiel Apr 5, 2022

skylerjokiel Apr 5, 2022

skylerjokiel Apr 5, 2022

markfields Apr 6, 2022

vladsud Apr 6, 2022

markfields left a comment

skylerjokiel left a comment

Add connect(), disconnect(), deprecate setAutoReconnect(), resume(), and connected #9439

Add connect(), disconnect(), deprecate setAutoReconnect(), resume(), and connected #9439

Conversation

scottn12 commented Mar 10, 2022 • edited Loading

msfluid-bot commented Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottn12 Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladsud Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

scottn12 Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottn12 Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

markfields left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markfields left a comment

Choose a reason for hiding this comment

skylerjokiel left a comment

Choose a reason for hiding this comment

Add `connect()`, `disconnect()`, deprecate `setAutoReconnect()`, `resume()`, and `connected` #9439

Add `connect()`, `disconnect()`, deprecate `setAutoReconnect()`, `resume()`, and `connected` #9439

scottn12 commented Mar 10, 2022 •

edited

Loading

msfluid-bot commented Mar 10, 2022 •

edited

Loading

scottn12 Mar 14, 2022 •

edited

Loading

vladsud Mar 11, 2022 •

edited

Loading

scottn12 Mar 11, 2022 •

edited

Loading

scottn12 Apr 5, 2022 •

edited

Loading